Skip to content

Fix sql port in admin tools when not provided#828

Merged
robholland merged 6 commits intotemporalio:mainfrom
efiShtain:fix-sql-port-in-admin-tools-when-not-provided
Jan 27, 2026
Merged

Fix sql port in admin tools when not provided#828
robholland merged 6 commits intotemporalio:mainfrom
efiShtain:fix-sql-port-in-admin-tools-when-not-provided

Conversation

@efiShtain
Copy link
Copy Markdown
Contributor

What was changed

This change try to set the correct SQL_PORT when using sql plugin and providing an address like my-db.endpoint.com without a port.
Current implementation sets the port to my-db.endpoint.com since it tries to use the last part after : and there is only one part

Why?

default port can be assumed and it is better than invalid port which is current implementation

  1. How was this tested:
    manually
  • without port, it takes the default store's port (5432 for postgres and 3306 for mysql)
  • with port it takes the correct one
  1. Any docs updates needed?
    not sure

@efiShtain efiShtain requested a review from a team as a code owner January 10, 2026 13:12
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jan 10, 2026

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@robholland
Copy link
Copy Markdown
Contributor

I'd prefer we fail the chart if they don't provide a port number, rather than us providing default ports. It is not unreasonable to require a port number.

@robholland robholland added the needs revision Team has requested some changes label Jan 14, 2026
@efiShtain
Copy link
Copy Markdown
Contributor Author

@robholland
I tend to agree, wanted to be less disruptive to current users

fixed it to fail if port isn't provided

let me know if any other changes are required

Comment thread charts/temporal/templates/_admintools-env.yaml Outdated
@robholland robholland removed the needs revision Team has requested some changes label Jan 27, 2026
@robholland robholland merged commit 3ced297 into temporalio:main Jan 27, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants